Fix ObjectDisposedException when disposing session after client.StopAsync()#479
Closed
SteveSandersonMS wants to merge 1 commit intomainfrom
Closed
Fix ObjectDisposedException when disposing session after client.StopAsync()#479SteveSandersonMS wants to merge 1 commit intomainfrom
SteveSandersonMS wants to merge 1 commit intomainfrom
Conversation
…sync() Make CopilotSession.DisposeAsync() idempotent and tolerant to already-disposed connections by adding an _isDisposed flag and catching ObjectDisposedException and IOException during disposal. Fixes #306 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SteveSandersonMS
deleted the
fix/dotnet-session-dispose-after-stop-resolved
branch
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #306 where disposing a CopilotSession after calling client.StopAsync() results in an ObjectDisposedException. The problem occurs because StopAsync() calls session.DisposeAsync() to clean up sessions, and then when the await using var session block exits, it calls DisposeAsync() again on an already-disposed connection.
Changes:
- Made
CopilotSession.DisposeAsync()idempotent using anInterlocked.Exchangepattern - Added exception handling to gracefully handle already-disposed connections
- Added regression test to verify the fix
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| dotnet/src/Session.cs | Added _isDisposed flag with thread-safe check and exception handling for ObjectDisposedException and IOException in DisposeAsync() |
| dotnet/test/ClientTests.cs | Added regression test Should_Not_Throw_When_Disposing_Session_After_Stopping_Client that reproduces the reported issue |
Cross-SDK Consistency ReviewThis PR fixes a .NET-specific issue where Consistency AnalysisAfter reviewing all four SDK implementations, I found that the other three SDKs have the same underlying issue but haven't exposed it yet: Current State Across SDKs:
Why This Matters:The bug surfaces when:
Go has the same risk because of session, _ := client.CreateSession(...)
defer session.Destroy() // Will be called even if Stop() is called first
// ... code ...
client.Stop() // Destroys all sessions, including this one
// When function exits, defer runs and tries to destroy againNode.js and Python are less affected because they don't have automatic cleanup patterns as common, but could still hit issues if users manually call RecommendationWhile this PR correctly fixes the .NET issue, consider adding similar defensive patterns to the other SDKs:
These changes would:
This PR is good to merge as-is for the .NET fix. The other SDKs could be addressed in follow-up PRs if desired.
|
|
Fix n changed this from a dirty hoe to what it was pose to be bitch hot n
std and claim i give to her lol dirty slut
…On Mon, Feb 16, 2026, 5:53 AM github-actions[bot] ***@***.***> wrote:
*github-actions[bot]* left a comment (github/copilot-sdk#479)
<#479 (comment)>
Cross-SDK Consistency Review
This PR fixes a .NET-specific issue where session.DisposeAsync() throws
ObjectDisposedException when called after client.StopAsync() has already
disposed the connection. The fix makes DisposeAsync() idempotent and
tolerant to disposed connections.
Consistency Analysis
After reviewing all four SDK implementations, I found that *the other
three SDKs have the same underlying issue* but haven't exposed it yet:
Current State Across SDKs:
SDK Cleanup Method Idempotent? Handles Connection Errors? Pattern
*.NET* (this PR) DisposeAsync() ✅ Yes ✅ Yes await using auto-calls dispose
*Node.js* destroy() ❌ No ❌ No Manual cleanup
*Python* destroy() ❌ No ❌ No Manual cleanup
*Go* Destroy() ❌ No ❌ No defer auto-calls Why This Matters:
The bug surfaces when:
1. User declares a session with automatic cleanup (await using in
.NET, defer in Go)
2. User calls client.stop() before the cleanup runs
3. The automatic cleanup tries to destroy an already-destroyed session
*Go has the same risk* because of defer patterns:
session, _ := client.CreateSession(...)defer session.Destroy() // Will be called even if Stop() is called first
// ... code ...client.Stop() // Destroys all sessions, including this one// When function exits, defer runs and tries to destroy again
*Node.js and Python are less affected* because they don't have automatic
cleanup patterns as common, but could still hit issues if users manually
call destroy() twice or after stop().
Recommendation
While this PR correctly fixes the .NET issue, consider adding similar
defensive patterns to the other SDKs:
1. *Go* - Make Destroy() idempotent and ignore connection errors
(similar to .NET fix)
2. *Node.js* - Make destroy() idempotent (less urgent since no
auto-cleanup, but good practice)
3. *Python* - Make destroy() idempotent (less urgent, but would enable async
with context manager support in the future)
These changes would:
- Prevent potential crashes in Go when using defer session.Destroy()
- Follow the [IAsyncDisposable contract]((learn.microsoft.com/redacted)
pattern across all languages
- Make the SDKs more resilient to cleanup ordering issues
*This PR is good to merge as-is* for the .NET fix. The other SDKs could
be addressed in follow-up PRs if desired.
AI generated by SDK Consistency Review Agent
<https://github.com/github/copilot-sdk/actions/runs/22059724281>
—
Reply to this email directly, view it on GitHub
<#479 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/B4HEF3P4OIOYZQ63VTQSUVT4MGORTAVCNFSM6AAAAACVISZHV6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTSMBXG44DANRSGM>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Supersedes #371 (resolves merge conflicts against current main).
Fixes #306
When
await client.StopAsync()is called before theawait using var sessiongoes out of scope,session.DisposeAsync()is called twice:StopAsync()(while connection is alive) ✓await usingcleanup (connection is now disposed) ✗ →ObjectDisposedExceptionSolution
Make
CopilotSession.DisposeAsync()idempotent and tolerant to already-disposed connections:_isDisposedflag - Thread-safe usingInterlocked.ExchangeDisposeAsyncidempotent - Returns immediately if already disposed (required byIAsyncDisposablecontract)ObjectDisposedExceptionandIOExceptionsince dispose methods should never throwThis follows the same pattern already used in
CopilotClient.DisposeAsync().Changes
dotnet/src/Session.cs: Add idempotency and exception handling toDisposeAsync()dotnet/test/ClientTests.cs: Add regression testCredit to @parthtrivedi2492 for the original PR #371.